CLDSRV-881: fix missing CORS headers on 403 responses#6126
CLDSRV-881: fix missing CORS headers on 403 responses#6126tcarmet wants to merge 8 commits intodevelopment/9.2from
Conversation
Hello tcarmet,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
LGTM |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.2 #6126 +/- ##
===================================================
+ Coverage 84.29% 84.40% +0.11%
===================================================
Files 204 204
Lines 13151 13174 +23
===================================================
+ Hits 11086 11120 +34
+ Misses 2065 2054 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
d7eec24 to
f073231
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
LGTM |
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
f073231 to
66233f9
Compare
|
LGTM |
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
66233f9 to
196a998
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
LGTM — the callback-wrapping approach in api.js is clean: it only pays the extra metadata lookup on error+Origin, guards headersSent, and correctly short-circuits on success. Two minor test quality notes posted inline. |
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
2 times, most recently
from
44b8aa0 to
406c33e
Compare
Cross-origin browser clients lost Access-Control-* headers whenever cloudserver returned 403, even though the bucket had a matching CORS rule. Responses now include the CORS headers for error responses on buckets with CORS configured, matching AWS behavior.
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
406c33e to
bf9577a
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
|
Clean, well-structured fix. The wrapper approach correctly limits the extra metadata lookup to error + Origin scenarios, and the fast-path check via hasCorsHeaders avoids redundant lookups when handlers already computed CORS headers. |
tcarmet
force-pushed
the
bugfix/CLDSRV-881-cors-headers-on-403-responses
branch
from
5d9b95f to
5ce3f67
Compare
|
LGTM — clean approach wrapping the callback at the |
|
LGTM |
bucketGetCors and bucketGetWebsite were passing corsHeaders at callback position 1 on AccessDenied, but both endpoints are routed through responseXMLBody which reads corsHeaders at position 2. The headers were silently dropped by the route layer, so 403 responses arrived without them.
|
LGTM |
| // We only pay the extra metadata lookup when an Origin header is present, | ||
| // matching AWS behavior and the existing contract of collectCorsHeaders. | ||
| function wrapCallbackWithErrorCorsHeaders(request, response, log, callback) { | ||
| return (err, ...rest) => { |
There was a problem hiding this comment.
If the handler computes corsHeaders and gets back {} (bucket has no CORS config), and then calls callback(err, null, {}), the wrapper will still do metadata.getBucket, right? (but it does not need to, I think).
Not sure if it's a good idea, but we could cache the bucket in request (something like request._loadedBucket) and first check there before metadata.getBucket
There was a problem hiding this comment.
Was able to confirm this with a extra test, indeed there would be 2 metadata call in this case.
So yeah we could consider {} as there's no header, and skip the bucket call in this case, it seems to be implicitly true, or we cache the bucket as you stated.
I'm debating between both right now, I pushed a code with the bucket cache on request. seems like the safest option between the two. Let me know what you think.
There was a problem hiding this comment.
Something I didn't consider when I suggested the cache approach: I think objectCopy and objectPutCopyPart call standardMetadataValidateBucketAndObj twice (once for the destination, once for the source), so the second call will overwrite the first, and we may not get the headers for the bucket we want.
You could confirm with a test.
There was a problem hiding this comment.
yes confirmed with the tests, nice catch again. At the end I ended up droping that cache entirely and accepting up to 2 metadata.getBucket calls on the error path. I believe the trade-off is fine here because:
- This only fires on failing requests with an Origin header where the handler returned without surfacing CORS headers (no rule match, no CORS config, or handler dropped the bucket between waterfall steps. Small slice of overall traffic.
- Turns out going the other way and trusting
{}as "no CORS to apply" would have regressed handlers that drop the bucket internally their collectCorsHeaders runs with bucket=undefined and returns{}even when a matching CORS rule exists. We'd silently lose CORS headers we currently apply. - A smarter cache (map keyed by
bucketNamesoobjectCopydoesn't overwrite the destination with the source) would have preserved the optimization without the leak you flagged. So that is an option we could arguably go this way, but I honestly felt like it was a over-optimization for an error-path issue, so I went with the simpler code.
To sum things up I removed the cache, documented the behavior in case we decide to optimize in the future and added/adapted the test cases (objectCopy and limiting number of calls to getBucket) so that if we do decide to mess with it, we ensure what we discussed is not lost and will fail if we handle it badly.
bucketGetLocation had the same AccessDenied arity mismatch as bucketGetCors and bucketGetWebsite: the error branch passed corsHeaders at callback position 1, but the GET route reads position 2. Align it with the standard three-argument error callback used elsewhere.
When a handler has already loaded the bucket, the CORS error wrapper no longer re-fetches it to compute headers.
|
LGTM |
Drop the request-scoped bucket cache so CORS evaluation always uses the request's actual bucket. Pin the regression scenario with a guardrail test against objectCopy and document the 2-call ceiling on the wrapper.
|
LGTM |
3 participants
When a bucket has CORS configured, cloudserver was omitting
Access-Control-*headers from 403 responses. Browsers treat such responses as opaque and block the error from the web app, even though AWS S3 returns CORS headers on error responses when a matching rule exists.The existing
collectCorsHeadershelper only runs inside an API handler, after bucket metadata has been loaded. Auth failures from Vault return before any handler runs, so the route-level error response path sent no CORS headers.The fix wraps
callApiMethod's callback: on error, when the request has anOriginheader and a known bucket, it fetches the bucket's CORS config and sets the matchingAccess-Control-*headers directly on the HTTP response before propagating the error. The extra metadata lookup only happens on failed requests with anOriginheader - no cost for non-CORS traffic or successful requests.Fixes: CLDSRV-881